Skip to content
This repository was archived by the owner on Apr 12, 2024. It is now read-only.

feat($httpBackend): flush requests in desired order #14967

Closed
wants to merge 2 commits into from
Closed

feat($httpBackend): flush requests in desired order #14967

wants to merge 2 commits into from

Conversation

dizel3d
Copy link
Contributor

@dizel3d dizel3d commented Jul 29, 2016 •
edited
Loading

What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)

feature

What is the current behavior? (You can also link to an open issue here)
#13717 We are currently unable to test what happens if two HTTP requests complete in a different order with the current API.

What is the new behavior (if this is a feature change)?

Now $httpBackend.flush has the second parameter start. So you can flush any range of pending requests array. Hence you can flush requests in a different order.

Does this PR introduce a breaking change?

No.

Please check if the PR fulfills these requirements

Other information:

It was impossible to flush pending requests in desired order.
Now it is possible because you can specify a start number.

@dizel3d
feat($httpBackend): flush requests in desired order
1b5025e 
It was impossible to flush pending requests in desired order.
Now it is possible because you can specify a `start` number.
@gkalpak gkalpak added this to the Backlog milestone Jul 31, 2016
gkalpak
gkalpak reviewed Jul 31, 2016
View reviewed changes
src/ngMock/angular-mocks.js
@@ -1788,24 +1788,29 @@ function createHttpBackendMock($rootScope, $timeout, $delegate, $browser) {
* @ngdoc method
* @name $httpBackend#flush
* @description
* Flushes all pending requests using the trained responses.
* Flushes pending requests using the trained responses in the order they arrived.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would mention the start/skip parameter in the description.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, I'll do.
(I am not a native speaker so I will be thankful to get any idea about the text.)

@gkalpak
Copy link
Member

gkalpak commented Jul 31, 2016

I think this is a nice addition. Left some (mostly) minor comments.
@dizel3d, feel free to ping me when/if you have addressed them, so I can take another look.

@dizel3d
Copy link
Contributor Author

dizel3d commented Jul 31, 2016

@gkalpak, I've made changes.

@Narretz
Copy link
Contributor

Narretz commented Aug 1, 2016

This introduces a breaking change as "skip" is now the second argument, no?

@dizel3d
Copy link
Contributor Author

dizel3d commented Aug 1, 2016

@Narretz, do you mean due to the changes digest became 3rd argument? Yes, it did. But digest is undocumented and used internally only.

@gkalpak
Copy link
Member

gkalpak commented Aug 1, 2016 •
edited
Loading

Only count was a documented parameter. The previously second (now third) digest parameter was undocumented/private, so nobody should be using it (except us). I don't consider this a breaking change technically, but it can break apps that are trying to be smart and take advantage of undocumented/private stuff.

Since this is not critical (i.e. it is an extra feature, not a fix), I am fine landing it in 1.6 only (but I wouldn't mark it as a breaking change).

@gkalpak
Copy link
Member

gkalpak commented Aug 1, 2016

LGTM (as long as Travis is happy).
@Narretz, what do you think about BC/landing branch?

@Narretz
Copy link
Contributor

Narretz commented Aug 1, 2016

Okay, since the digest parameter is private I'm fine with landing this in 1.5.
Btw, this is a duplicate of #6412

Can you add
Closes #13717
at the end of the commit message?

@Narretz Narretz mentioned this pull request Aug 1, 2016
Closed
@dizel3d
Copy link
Contributor Author

dizel3d commented Aug 1, 2016

@Narretz, actually the PR closes only paragraph 2 of #13717.
(The PR contains 2 commits. In which should I add Closes #13717?)

@Narretz
Copy link
Contributor

Narretz commented Aug 1, 2016

@dizel3d Then just add Related #13717 to any of the 2 commits. We'll squash them together on merge.

@dizel3d
feat($httpBackend): flush requests in desired order
97ef447 
Made changes according to results of PR review.

Related angular#13717
@dizel3d dizel3d force-pushed the feat-http-backend-flush-order branch from 1bcb28f to 97ef447 Compare August 1, 2016 18:53
@dizel3d
Copy link
Contributor Author

dizel3d commented Aug 1, 2016

@Narretz Done!

@Narretz
Copy link
Contributor

Narretz commented Aug 2, 2016

LGTM

@gkalpak gkalpak closed this in 72b6632 Aug 8, 2016
gkalpak pushed a commit that referenced this pull request Aug 8, 2016
@dizel3d @gkalpak
feat(ngMock/$httpBackend): flush requests in any order
098b6f5 
Previously, requests were flushed in the order in which they were made.
With this change, it is possible to flush requests in any order. This is useful for simulating more
realistic scenarios, where parallel requests may be completed in any order.

Partially addresses #13717.

Closes #14967
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers
No reviews
Assignees
No one assigned
Labels
cla: yes component: $http component: ngMock frequency: low needs: work severity: inconvenient type: feature
Projects
None yet
Milestone
Backlog - won't fix
Development

Successfully merging this pull request may close these issues.
4 participants
@dizel3d @gkalpak @Narretz @googlebot